Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Serial ID device target filter #84

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

tomas-pecserke
Copy link

@tomas-pecserke tomas-pecserke commented Jun 2, 2023

This PR is in response to #54 and improves usability of PR #83. This feature can be further improved by integrating the ability to read EEPROM serial number. This functionality is implemented in PR #86.

It adds --serial <serial> filter flag to device selection. The serial ID is then compared with string descriptor specified by iSerial device configuration option.

As the device in bootloader mode uses different serial than in firmware, changes need to made to filter setting in order to find the device after forced reset. If --serial option is specified and the device is forced to reboot by the selected command, filters are adjusted to match the device by combination of bus and port number of the device before the reboot.

As per libusb docs for libusb_get_port_number:

Unless the OS does something funky, or you are hot-plugging USB extension cards, the port number returned by this call is usually guaranteed to be uniquely tied to a physical port, meaning that different devices plugged on the same physical port should return the same port number.

This should be fine for matching immediately after automatic reboot for most cases.

This solution does NOT break backward compatibility - functionality and outputs do not change if the flag is not specified.

Usage:

picotool info --serial ABCDEFG -f
picotool load -x program.u2f --serial ABCDEFG -f

For device matching after the reboot, I use internal USB port number filter. This filter can be easily exposed if considered useful for general use. I did not expose it right away to prevent feature creep and CLI option pollution without prior discussion.

@tomas-pecserke tomas-pecserke force-pushed the feature/serial-filter branch 3 times, most recently from 3083dc8 to d33a555 Compare June 3, 2023 03:18
@tomas-pecserke tomas-pecserke changed the title Add Serial ID device filter Add Serial ID device target filter Jun 3, 2023
@Ferdi265
Copy link

Ferdi265 commented Jun 3, 2023

Thanks for implementing that.

Please note that the way you implemented this (just taking the libusb device serial number) is not extremely useful for flashing:

Case 1: Two picos are in bootloader mode

In this case, both picos will present the same libusb device serial number, and you will not be able to select a specific one of them with --serial

Case 2: Two picos are connected and not in bootloader mode

In this case, rebooting one of them and flashing it in one go with -f will not work, since the serial number presented changes and picotool will not be able to find the device after the reboot.

One workaround for this would be to remove the --serial filter on reboot when using -f, similar to the --bus and --addr filters. This would fix Case 2 as long as only one pico is in bootloader mode at the same time.

The only way to fix this completely is to extract the real serial number from devices in bootloader mode, which as I currently understand it requires execution of extractor code on the target pico as implemented in #80. (Which might not be desireable)

A variant of --serial that fixes both cases is available in my rewrite-main branch of my personal fork of picotool, though that branch changes a bunch more and probably is not immediately mergeable.

@Ferdi265
Copy link

Ferdi265 commented Jun 3, 2023

TL;DR: if -f is fixed to work with --serial, I'm 100% in favor of this PR being merged ASAP.

This is a much simpler solution to mine, and makes serial filters usable for most purposes.

@lurch
Copy link
Contributor

lurch commented Jun 3, 2023

This should probably be targeting the develop branch?

@tomas-pecserke
Copy link
Author

@Ferdi265: Thanks for pointing this out to me.

Case 1: Two picos are in bootloader mode

I never knew the bootloader serial ID is not unique - I tested it on 2 board and each reported different serial - when I looked into it now, it seems each has different version of bootloader. I meant to address this with making --serial option repeatable (still might be useful), but clipp is giving me trouble.

I wonder if using the vendor device to read EEPROM serial id like in stdio_usb using FLASH_RUID_CMD 0x4b is feasible in the bootloader.

Case 2: Two picos are connected and not in bootloader mode

I could try to use the bus and address before the restart to narrow the search after the restart - the new device should be in the same bus/port but with different address.

@Ferdi265
Copy link

Ferdi265 commented Jun 3, 2023

AFAIK, the bootrom command interface does not allow you to insert arbitrary SPI commands, so you can't use FLASH_RUID_CMD directly. The best way I found for now is to upload code into RAM that then reads out the serial using FLASH_RUID_CMD (see the code from my PR for that), not exactly a clean solution IMO.

@tomas-pecserke
Copy link
Author

Yes, I looked at your code once I discovered your issue. I don't like the idea of code execution too, but it might be made safer based on knowledge of the bootrom code and version and only ever executed on devices with its specific VID/PID and known bootrom serial.

I am now thinking it might be better to implement support for DFU load / save. That's where this filer would shine.

@Ferdi265
Copy link

Ferdi265 commented Jun 3, 2023

You might want to look at my rewrite-main branch; that contains a bunch more changes and simplifies work with multiple picos by a lot. It is probably not production-ready, but might be an interesting inspiration.

@tomas-pecserke tomas-pecserke changed the base branch from master to develop June 3, 2023 17:47
@tomas-pecserke tomas-pecserke changed the title Add Serial ID device target filter [draft] Add Serial ID device target filter Jun 3, 2023
@tomas-pecserke
Copy link
Author

tomas-pecserke commented Jun 3, 2023

Yes, that's I meant by looking at you code 👍. I see you extracted the part dealing with extracting EEPROM serial into #86. When it gets merged, I will likely base this filter on it.

For now, I think the path of least resistance, least duplicated effort, and most immediate usefulness means changing this filter to accept any device in bootrom after forced reboot.

I also try to match it based on bus topology and position of the matched device prior to forced reboot.

@tomas-pecserke tomas-pecserke changed the title [draft] Add Serial ID device target filter Add Serial ID device target filter Jun 3, 2023
main.cpp Outdated Show resolved Hide resolved
@will-v-pi will-v-pi added this to the 1.7.0 milestone Jul 26, 2024
@DavidEGrayson
Copy link
Contributor

DavidEGrayson commented Sep 1, 2024

In get_usb_device_serial I see you are connecting to the device (which could fail if the device is open by another process), and performing I/O to get its serial number. I just thought I'd point out that it's possible to get that serial number without connecting or doing I/O, because the OS records the serial number at enumeration time and provides an API for getting it. I implemented that in libusbp, which supports Windows, Linux, and macOS. You can compile the lsusb example from that library if you want to see it working. I really don't understand why libusb doesn't have this feature, but it doesn't.

Also, I just tested a Pico 2, and it appears that the serial number for the bootloader is the same as the firmware! So we could just say that --serial does not work properly for the RP2040 since its bootloader does not have proper serial numbers, but it does work on the RP2350 chips, and then we don't have to mess around with port numbers or any other way of identifying a chip.

@Ferdi265
Copy link

Ferdi265 commented Sep 1, 2024

In get_usb_device_serial I see you are connecting to the device (which could fail if the device is open by another process), and performing I/O to get its serial number. I just thought I'd point out that it's possible to get that serial number without connecting or doing I/O, because the OS records the serial number at enumeration time and provides an API for getting it. I implemented that in libusbp, which supports Windows, Linux, and macOS. You can compile the lsusb example from that library if you want to see it working. I really don't understand why libusb doesn't have this feature, but it doesn't.

Also, I just tested a Pico 2, and it appears that the serial number for the bootloader is the same as the firmware! So we could just say that --serial does not work properly for the RP2040 since its bootloader does not have proper serial numbers, but it does work on the RP2350 chips, and then we don't have to mess around with port numbers or any other way of identifying a chip.

As you noticed, the USB serial number in bootloader mode is the same for all devices on rp2040. Having the serial check work for rp2040 even when in bootloader mode is very useful and should be there, otherwise flashing multiple rp2040s becomes a pain very fast.

The suggestion with libusbp sounds good though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants